Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export local backend middlewares in proxy server #5852

Closed
wants to merge 1 commit into from

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Sep 30, 2021

Summary

For local_backend users with an existing express server already running, who would like to avoid having to run yet another server on another port to have some parts of the functionality work. With this change, it's possible to use the middleware directly, without creating a whole server in a new process:

const handler = require('netlify-cms-proxy-server/dist/middlewares').localFsMiddleware({
  repoPath: ...,
  logger: console,
})

handler(req, res)

👆 the above works both with express and in a next.js api handler.

Test plan

So, my plan was just to export it to begin with. My hope was that it could be an undocumented part of the API surface of this library and therefore subject to change without a major bump, but it would be useable for those who happen to know it's there.

Having it be an official part of the "real" API surface would be a comparatively change: there would need to be a separate entrypoint for the middleware module part of the library, which doesn't have the side-effect of trying to start a server (and it'd probably be a good idea to generate .d.ts files and write documentation, recommended usage, etc., and maybe get rid of the type dependency on winston in the params). And I can't see the harm in just doing this small change for now.

Checklist

  • I have read the contribution guidelines.
  • Code is formatted via running yarn format.
  • Tests are passing via running yarn test.
  • The status checks are successful (continuous integration). Those can be seen below.

@mmkal mmkal requested a review from a team September 30, 2021 03:50
@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Sep 30, 2021
Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mmkal, thanks for the PR ❤️ We already export methods to register a specific middleware, see #3361.
Could you use that approach?

Also, the approach in your PR is missing some init logic to setup schema validation and other required middlewares like CORS and JSON parsing. See registerCommonMiddlewares

@mmkal
Copy link
Contributor Author

mmkal commented Sep 30, 2021

Hey @erezrokah, all very fair points. I was taking a shortcut to do the bare minimum possible to get it working with next.js - i.e. in a context where there is no app with .post or .use methods, so traditional middleware basically isn't supported. But yes it does mean there's no schema validation, cors, json-parsing or logging. Those were all ok to miss because respectively, the client is only sending valid requests, we happen to not need cors, next.js has already done json-parsing for us, and we are able to do our own logging.

I think I could probably make it work with the existing exports by passing in a fake express app from the handler, but that would be quite brittle.

An alternative - could the dist folder be generated using tsc rather than webpack? Given it's a node library/CLI I can't see why it would need to be bundled or minified. There'd be a few benefits:

  • users could import from dist/middlewares/whatever, at their own risk
  • type declarations would come for free
  • no need to maintain the webpack config anymore! (This is only a benefit because you already have a tsconfig, otherwise you'd be swapping one for the other)

@mmkal
Copy link
Contributor Author

mmkal commented Sep 30, 2021

Update: got it working with the existing exports by passing in a fake app. Behold the hoop-jumping:

export default async (req, res) => {
  const netlifyCmsProxyServer =
    require('netlify-cms-proxy-server/dist/middlewares') as typeof import('netlify-cms-proxy-server/src/middlewares')
  const path = require('path')
  const handlers: Function[] = []
  process.env.GIT_REPO_DIRECTORY = path.join(process.cwd(), '../..')

  const fakeApp = {
    use: () => {}, // skip next.js 
    post: (_path: string, handler: Function) => handlers.push(handler),
  }
  
  await netlifyCmsProxyServer.registerLocalFs(fakeApp as any, { logLevel: 'error' })
  
  for (const handler of handlers) {
    let nextCalled = false
    const next = () => (nextCalled = true)
    await handler(req, res, next)
    if (!nextCalled) {
      break
    }
  }
}

The above could easily break if the library changes some parts of its implementation, so I'd still be in favour of exporting the extra functions. But I might play around with a tsc build alternative instead.

@erezrokah
Copy link
Contributor

Closing in favor of #5856. I do believe using tsc is the right approach, given that we don't break existing consumers (we'll need to verify).

@erezrokah erezrokah closed this Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants